bpo-45385: Fix reference leak from descr_check by corona10 · Pull Request #28719 · python/cpython

corona10 · 2021-10-04T09:43:41Z

motivation:

GH detects the compiler warnings while executing the CI pipeline.
Fix possible reference leak for the method_check_args use-case.

https://bugs.python.org/issue45385

vstinner

Your commit title is misleading. What your PR does it is to fix compiler warnings in Objects/descrobject.c. It's unrelated to GitHub Action. It's just that you noticed them thanks for GHA.

vstinner · 2021-10-04T09:47:28Z

Maybe descr_check() should set res to NULL when it returns 0, whereas having to modify all functions using it.

If you modify descr_check() for that, maybe add a comment to explain that it's only set to NULL to prevent compiler warnings.

vstinner

motivation: #28572

Can you please mention the fixed compiler warning in the commit message?

vstinner · 2021-10-04T09:57:56Z

motivation: #28572

I don't see the relationship between your PR and the PR #28572.

corona10 · 2021-10-04T10:04:58Z

I don't see the relationship between your PR and the PR #28572.

I detected the warning from the PR but I will update the content not to mention the PR :)

corona10 · 2021-10-04T10:07:01Z

@vstinner
Thanks Victor for review :)
Is this what you intended?

corona10 · 2021-10-04T10:25:00Z

comparison of integer expressions of different signedness: ‘long unsigned int’ and ‘int’ [-Wsign-compare]

Will be fixed at #28716

Fidget-Spinner

LGTM in general. Thank you!

corona10 · 2021-10-06T02:36:36Z

@vstinner @Fidget-Spinner
PTAL, this PR will fix both compiler warning and possible ref leak :)

vstinner · 2021-10-06T10:06:31Z

Would you mind to use Py_NewRef()?

I don't think we can, the bugfix needs to be backported to 3.9. Py_NewRef was added in 3.10. Unless you mean Py_NewRef for 3.11, 3.10 and manual backport for 3.9.

3.10 backport should not prevent to write better code in the main branch. The backport can be different.

@vstinner @Fidget-Spinner
I will backport it manually :)

Fidget-Spinner

LGTM. Just needs a news IMO.

Fidget-Spinner · 2021-10-06T10:14:35Z

Wow, why did this (and the code below) change too? I don't think this was buggy, though the new code improves readability/makes it easier to understand.

I'm guessing for consistency reasons?

I asked @corona10 to mention compiler warnings in the commit message. I cannot see compiler warnings in https://bugs.python.org/issue45385, in the PR description, or in the commit message.

I'm guessing for consistency reasons?

Yes and it improve the readability :)

vstinner · 2021-10-06T12:25:25Z

There is no need to cast the Py_NewRef() argument to PyObject*.

vstinner

LGTM. But please mention the fixed compiler warnings in the commit message and/or in the bpo.

"Fix possible reference leak". Either there is a leak or not leak. I suggest to remove "possible".

bedevere-bot · 2021-10-06T14:55:44Z

🤖 New build scheduled with the buildbot fleet by @Fidget-Spinner for commit 336434c 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

miss-islington · 2021-10-07T00:51:59Z

Thanks @corona10 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

bedevere-bot · 2021-10-07T00:52:00Z

@corona10: Please replace # with GH- in the commit message next time. Thanks!

bedevere-bot · 2021-10-07T00:52:13Z

GH-28779 is a backport of this pull request to the 3.10 branch.

(cherry picked from commit e6ff4eb) Co-authored-by: Dong-hee Na <donghee.na@python.org>

bedevere-bot · 2021-10-07T01:25:12Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot x86-64 macOS 3.x has failed when building commit e6ff4eb.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/366/builds/1003) and take a look at the build logs.
Check if the failure is related to this commit (e6ff4eb) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/366/builds/1003

Failed tests:

test_importlib

Failed subtests:

test_multiprocessing_pool_circular_import - test.test_importlib.test_threaded_import.ThreadedImportTests

Summary of the results of the build (if available):

== Tests result: FAILURE then FAILURE ==

408 tests OK.

10 slowest tests:

test_concurrent_futures: 4 min 29 sec
test_multiprocessing_spawn: 4 min
test_unparse: 3 min 26 sec
test_tokenize: 3 min 17 sec
test_lib2to3: 2 min 29 sec
test_multiprocessing_forkserver: 2 min 17 sec
test_unicodedata: 2 min 9 sec
test_asyncio: 1 min 57 sec
test_capi: 1 min 41 sec
test_pickle: 1 min 15 sec

1 test failed:
test_importlib

18 tests skipped:
test_devpoll test_epoll test_gdb test_ioctl test_msilib
test_multiprocessing_fork test_ossaudiodev test_smtpnet test_spwd
test_ssl test_startfile test_tix test_tk test_ttk_guionly
test_winconsoleio test_winreg test_winsound test_zipfile64

1 re-run test:
test_importlib

Total duration: 29 min 19 sec

Click to see traceback logs

Traceback (most recent call last):
  File "/Users/buildbot/buildarea/3.x.billenstein-macos/build/Lib/multiprocessing/resource_tracker.py", line 209, in main
    cache[rtype].remove(name)
    ^^^^^^^^^^^^^^^^^^^^^^^^^
KeyError: '/psm_660b9f0f'


Traceback (most recent call last):
  File "/Users/buildbot/buildarea/3.x.billenstein-macos/build/Lib/test/test_importlib/test_threaded_import.py", line 258, in test_multiprocessing_pool_circular_import
    script_helper.assert_python_ok(fn)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/buildbot/buildarea/3.x.billenstein-macos/build/Lib/test/support/script_helper.py", line 160, in assert_python_ok
    return _assert_python(True, *args, **env_vars)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/buildbot/buildarea/3.x.billenstein-macos/build/Lib/test/support/script_helper.py", line 145, in _assert_python
    res.fail(cmd_line)
    ^^^^^^^^^^^^^^^^^^
  File "/Users/buildbot/buildarea/3.x.billenstein-macos/build/Lib/test/support/script_helper.py", line 72, in fail
    raise AssertionError("Process return code is %d\n"
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: Process return code is 1
command line: ['/Users/buildbot/buildarea/3.x.billenstein-macos/build/python.exe', '-X', 'faulthandler', '-I', '/Users/buildbot/buildarea/3.x.billenstein-macos/build/Lib/test/test_importlib/partial/pool_in_threads.py']


Traceback (most recent call last):
  File "/Users/buildbot/buildarea/3.x.billenstein-macos/build/Lib/multiprocessing/resource_tracker.py", line 209, in main
    cache[rtype].remove(name)
    ^^^^^^^^^^^^^^^^^^^^^^^^^
KeyError: '/psm_660c8d6e'

(cherry picked from commit e6ff4eb) Co-authored-by: Dong-hee Na <donghee.na@python.org>

…8780) (cherry picked from commit e6ff4eb) Co-authored-by: Dong-hee Na <donghee.na@python.org>

bedevere-bot added the awaiting core review label Oct 4, 2021

the-knights-who-say-ni added the CLA signed label Oct 4, 2021

corona10 added skip issue skip news and removed CLA signed awaiting core review labels Oct 4, 2021

the-knights-who-say-ni added the CLA signed label Oct 4, 2021

corona10 requested a review from Fidget-Spinner October 4, 2021 09:43

vstinner reviewed Oct 4, 2021

View reviewed changes

corona10 changed the title ~~no-issue: Remove compiler warning from Github Action.~~ no-issue: Fix compiler warnings in Objects/descrobject.c. Oct 4, 2021

corona10 force-pushed the nit branch from fdd93ec to 46c9bcb Compare October 4, 2021 09:49

corona10 changed the title ~~no-issue: Fix compiler warnings in Objects/descrobject.c.~~ no-issue: Fix compiler warnings in Objects/descrobject.c Oct 4, 2021

corona10 requested a review from vstinner October 4, 2021 10:06

corona10 commented Oct 4, 2021

View reviewed changes

Comment thread Objects/descrobject.c Outdated

corona10 Oct 4, 2021

Copy link
Copy Markdown

Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vstinner
Thanks Victor for review :)
Is this what you intended?

Fidget-Spinner reviewed Oct 4, 2021

View reviewed changes

Comment thread Objects/descrobject.c Outdated

Comment thread Objects/descrobject.c Outdated

corona10 requested a review from Fidget-Spinner October 4, 2021 12:33

vstinner reviewed Oct 4, 2021

View reviewed changes

Comment thread Objects/descrobject.c Outdated

Comment thread Objects/descrobject.c Outdated

Comment thread Objects/descrobject.c Outdated

corona10 requested a review from vstinner October 4, 2021 15:13

Fidget-Spinner reviewed Oct 5, 2021

View reviewed changes

Comment thread Objects/descrobject.c Outdated

corona10 changed the title ~~no-issue: Fix compiler warnings in Objects/descrobject.c~~ bpo-45385: Fix compiler warnings in Objects/descrobject.c Oct 6, 2021

corona10 changed the title ~~bpo-45385: Fix compiler warnings in Objects/descrobject.c~~ bpo-45385: Fix possible reference leak from descr_check Oct 6, 2021

corona10 requested a review from Fidget-Spinner October 6, 2021 01:00

corona10 force-pushed the nit branch from 029ec47 to 4e030a6 Compare October 6, 2021 02:29

corona10 removed the skip issue label Oct 6, 2021

corona10 added the needs backport to 3.10 only security fixes label Oct 6, 2021

vstinner reviewed Oct 6, 2021

View reviewed changes

Fidget-Spinner reviewed Oct 6, 2021

View reviewed changes

corona10 removed the needs backport to 3.9 label Oct 6, 2021

vstinner reviewed Oct 6, 2021

View reviewed changes

corona10 requested a review from vstinner October 6, 2021 14:04

vstinner approved these changes Oct 6, 2021

View reviewed changes

bedevere-bot added the awaiting merge label Oct 6, 2021

corona10 changed the title ~~bpo-45385: Fix possible reference leak from descr_check~~ bpo-45385: Fix reference leak from descr_check Oct 6, 2021

bpo-45385: Fix reference leak from descr_check

336434c

corona10 force-pushed the nit branch from b8adfff to 336434c Compare October 6, 2021 14:52

Fidget-Spinner added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 6, 2021

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 6, 2021

corona10 self-assigned this Oct 6, 2021

Fidget-Spinner approved these changes Oct 6, 2021

View reviewed changes

corona10 merged commit e6ff4eb into python:main Oct 7, 2021

bedevere-bot removed awaiting merge needs backport to 3.10 only security fixes labels Oct 7, 2021

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 7, 2021

bpo-45385: Fix reference leak from descr_check (pythonGH-28719)

e4f17f2

(cherry picked from commit e6ff4eb) Co-authored-by: Dong-hee Na <donghee.na@python.org>

corona10 deleted the nit branch October 7, 2021 00:52

corona10 added a commit to corona10/cpython that referenced this pull request Oct 7, 2021

[3.9] bpo-45385: Fix reference leak from descr_check (pythonGH-28719)

1485367

(cherry picked from commit e6ff4eb) Co-authored-by: Dong-hee Na <donghee.na@python.org>

corona10 added a commit that referenced this pull request Oct 7, 2021

[3.9] bpo-45385: Fix reference leak from descr_check (GH-28719) (GH-2…

660718d

…8780) (cherry picked from commit e6ff4eb) Co-authored-by: Dong-hee Na <donghee.na@python.org>

corona10 pushed a commit that referenced this pull request Oct 7, 2021

bpo-45385: Fix reference leak from descr_check (GH-28719) (GH-28779)

35d4857

Uh oh!

Uh oh!

Conversation

corona10 commented Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Oct 4, 2021

Uh oh!

corona10 commented Oct 4, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corona10 commented Oct 4, 2021

Uh oh!

Fidget-Spinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

corona10 commented Oct 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fidget-Spinner Oct 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fidget-Spinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Oct 6, 2021

Uh oh!

miss-islington commented Oct 7, 2021

Uh oh!

bedevere-bot commented Oct 7, 2021

Uh oh!

bedevere-bot commented Oct 7, 2021

Uh oh!

bedevere-bot commented Oct 7, 2021

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

corona10 commented Oct 4, 2021 •

edited

Loading

Fidget-Spinner Oct 6, 2021 •

edited

Loading